Skip to content

fix(python): trim Layer.source redaction cost and document save_project - #1772

Merged
giswqs merged 2 commits into
mainfrom
fix/python-layer-source-redaction-cost
Aug 8, 2026
Merged

fix(python): trim Layer.source redaction cost and document save_project#1772
giswqs merged 2 commits into
mainfrom
fix/python-layer-source-redaction-cost

Conversation

@giswqs

@giswqs giswqs commented Aug 8, 2026

Copy link
Copy Markdown
Member

Summary

Follow-up to #1770, which merged while the last round of review comments was still being addressed. Three items from that round:

  • Layer.source went through project.redact_layer, which deep-copies the whole layer record, including an inlined GeoJSON blob, only to keep the small source sub-object. It now sweeps just that field via a new project.redact_layer_field. Layer.data still copies the record whole, which is what it is for and is documented as such.
  • The top-level geolibre.save_project writes verbatim, credentials included, while Map.save_project redacts by default. That is deliberate, since the MCP server round-trips a user's own project file through it and redacting on every edit would strip their own API keys, but nothing said so. Documented on the function and in the README, pointing at Map.save_project and redact_credentials for the sharing case.
  • docs/python.md's Map API table still listed only the pre-feat(python): expand layer management and headless camera API #1770 methods, so it had drifted from the expanded table in python/README.md. Synced.

Test plan

  • cd python && pytest (314 passed, 3 skipped, with the mcp extra installed)
  • ruff check and ruff format --check clean over python/src/geolibre/

Summary by CodeRabbit

  • New Features
    • Added a helper to create credential-scrubbed copies of individual layer configuration fields.
  • Bug Fixes
    • Improved layer source handling so credentials are removed without altering unrelated layer information.
  • Documentation
    • Expanded guidance for project and layer management, camera state, and layer operations.
    • Clarified layer removal identifiers and updated the remove_layer parameter name.
    • Documented credential handling for project saves and recommended redacting data before sharing.

- Sweep only the source field in Layer.source. redact_layer copied an inlined
  geojson blob first and then discarded it, which .data pays for deliberately
  but .source has no reason to.
- Document that the top-level save_project writes verbatim, credentials
  included, and why: it is the lossless primitive the MCP server round-trips a
  user's own file through, where redacting on every edit would strip their own
  API keys. Point at Map.save_project and redact_credentials for the sharing
  case.
- Sync docs/python.md's Map API table with the methods this PR adds; it had
  drifted from the expanded table in python/README.md.
Copilot AI lite review requested due to automatic review settings August 8, 2026 05:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d36bb383-ad64-4233-a1f5-e274d6f07358

📥 Commits

Reviewing files that changed from the base of the PR and between 5f738e5 and bd0165a.

📒 Files selected for processing (1)
  • python/README.md

📝 Walkthrough

Walkthrough

The PR adds field-level credential redaction for layer values. It updates Layer.source to use the helper. It documents project saving, credential handling, camera state, and layer management APIs.

Changes

Credential Redaction and API Documentation

Layer / File(s) Summary
Field-level credential redaction
python/src/geolibre/project.py, python/src/geolibre/geolibre.py
Adds redact_layer_field and uses it in Layer.source to sanitize only the source field.
Project saving and layer API documentation
python/src/geolibre/authoring.py, python/README.md, docs/python.md
Documents verbatim project saving, credential redaction, persisted camera state, layer management, and the corrected remove_layer parameter name.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: nyxst4ck

Poem

A rabbit cleaned one source field,
While other layer data stayed revealed.
Save projects whole, or redact with care,
Clear API notes now guide us there.
Hop, hop, safer docs to share!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the Python redaction optimization and save_project documentation updates, which are the main changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/python-layer-source-redaction-cost

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

🔍 Cloudflare PR preview

Item Value
Site https://5c821812.geolibre-preview.pages.dev
Demo app https://5c821812.geolibre-preview.pages.dev/demo/
Commit bd0165a

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Code review

I reviewed the diff (docs/python.md, python/README.md, python/src/geolibre/{authoring.py, geolibre.py, project.py}) against the surrounding source, including _redact_config, _sweep_layer_credentials, redact_layer, and the existing test coverage in python/tests/test_scripting.py (lines 666–731, which exercise Layer.source/Layer.data redaction).

Bugs: None found. redact_layer_field (project.py) is a straight extraction of the per-field sweep _sweep_layer_credentials already performs (layer[field] = _redact_config(layer[field])); _redact_config always rebuilds dicts/lists/leaves into new objects rather than mutating or aliasing its input, so calling it directly on self._layer().get("source") (geolibre.py) is behaviorally identical to the old redact_layer(self._layer()).get("source") — it neither leaks a live reference into the store nor changes what gets redacted. Confidence: high.

Security: No change in redaction coverage — source is still swept by the same credential-name/URL-scrubbing logic before being handed back to the caller. Confidence: high.

Performance: The stated goal (avoid deep-copying the whole layer, including a potentially large inlined GeoJSON blob, just to read source) is achieved correctly. Confidence: high.

Quality: All new/edited docstrings (authoring.py save_project, geolibre.py Layer.source) and the docs/README table additions accurately describe the current code — I verified every method listed in the new docs/python.md table row (set_zoom, set_bearing, set_pitch, fit_project_bounds, center/zoom/bearing/pitch/basemap/name, rename_layer, move_layer, duplicate_layer, show_layer, hide_layer, layer_properties, column_values, describe, remove_layer) exists with matching signatures/behavior. Minor, non-blocking nit: the new Note: docstring section in authoring.py is the only use of that Google-style section header in the package (elsewhere only Args/Returns/Raises appear) — purely stylistic, not worth a fix. Confidence: low.

CLAUDE.md: No applicable guidelines are implicated (change is confined to the Python package and docs, not the JS workspaces/Whitebox catalog/CSP/i18n areas CLAUDE.md calls out). Confidence: high.

No inline comments posted — nothing rose to a level warranting one.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/python.md`:
- Line 260: Update the remove_layer entry in the API table to use the parameter
name layer instead of layer_id, matching its supported ID, name, and Layer
handle selectors; leave clear_layers() and the description unchanged.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4cc43821-3bfd-4f10-95e6-f64186ec6d2f

📥 Commits

Reviewing files that changed from the base of the PR and between 81da469 and 5f738e5.

📒 Files selected for processing (5)
  • docs/python.md
  • python/README.md
  • python/src/geolibre/authoring.py
  • python/src/geolibre/geolibre.py
  • python/src/geolibre/project.py

Comment thread docs/python.md
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

🔍 GitHub Pages PR preview

Item Value
Site https://opengeos.org/pages-preview/GeoLibre/pr-1772/
Demo app https://opengeos.org/pages-preview/GeoLibre/pr-1772/demo/
Commit bd0165a

Note

GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating.

Fix the remove_layer row in python/README.md rather than docs/python.md. The two
tables disagreed and the README was the wrong one: the parameter is `layer_id`,
so `remove_layer(layer=...)` raises TypeError. Both now match the signature,
and the description already says it accepts an id, name, or handle.
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

All accurate. Everything checks out: the redact_layer_field refactor is behaviorally equivalent to the old redact_layer(...).get("source") path but avoids the wasted deep-copy of the whole layer (including any inlined GeoJSON), the docstring updates accurately describe the save_project verbatim-vs-redacted split, and the doc-table syncs match the real method/property signatures in geolibre.py. No test changes were needed since existing test_layer_data_and_source_redact_credentials already exercises layer.source redaction and continues to pass under the new implementation.

I found no bugs, security issues, or CLAUDE.md violations worth flagging as inline comments — this is a small, low-risk refactor + docs PR.

Code review

Bugs: None found. redact_layer_field is a straightforward extraction of the existing _redact_config call that redact_layer/_sweep_layer_credentials already performed on the source field; depth counting, detachment semantics, and the None/missing-field case all match the prior behavior. (Confidence: high)

Security: None found. Credential sweeping still goes through the same _redact_config logic; no new attack surface introduced. (Confidence: high)

Performance: The stated goal is met — Layer.source no longer deep-copies the whole layer record (including any inlined GeoJSON) just to discard everything but source. (Confidence: high)

Quality: Small, well-scoped change with a clear docstring explaining why the new helper exists; existing test (test_layer_data_and_source_redact_credentials) still covers the behavior. No new unit test targets redact_layer_field directly, but coverage via Layer.source is adequate for this size of change. (Confidence: medium)

CLAUDE.md: N/A — this PR touches only python/ and docs/python.md, no areas governed by the repo's other convention rules (Whitebox catalog, i18n, CSP, etc.). (Confidence: high)

@giswqs
giswqs merged commit 7238e82 into main Aug 8, 2026
26 checks passed
@giswqs
giswqs deleted the fix/python-layer-source-redaction-cost branch August 8, 2026 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants